-
-
Couldn't load subscription status.
- Fork 0
Add dialog demo #4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks good overall. I few minor things, and I think I'd like a Miriam/Stacy review as well!
| margin: unset; | ||
| text-align: center; | ||
| text-wrap: balance; | ||
| transition: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we also add a slight movement transition from the bottom? I think from bottom: -20px to bottom: 0px makes it feel natural.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 8216c8a
src/dialog/index.html
Outdated
| <article> | ||
| <h1 data-heading="main"> Popping up with the <code>dialog</code> element</h1> | ||
| <section> | ||
| <h2 data-heading> Some Jumping Spiders |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we do a play on JS = jumping spiders?
"Less JS (JavaScript), More JS (Jumping Spiders)"
"Vanilla JS (but the JS is Jumping spiders)"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about "Less JS, More Jumping Spiders"? 🕷️🕸️
✅ Deploy Preview for oddbaseline ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
src/dialog/style.css
Outdated
| transition: | ||
| opacity var(--transition-timing-slower) ease-in, | ||
| overlay var(--transition-timing-fast) linear, | ||
| display var(--transition-timing-fast) linear, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the out animation is jumpy because display animates so much faster than opacity. Consider making display one of the slowest, so it doesn't disappear before it's done fading?
|
@mirisuzanne I think I addressed all of the nesting and animation issues, so this is ready for another quick look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dvdherron looks great. There's a little linting that I think would clean up the code some, but isn't essential and we've never discussed any 'rules for nesting' or anything. To me it reads better if the nested stuff can be ordered:
- properties
- variants (like
&[open]) - descendants (like
[data-heading])
Since there's specificity differences at play, I think that can all be reordered safely without impacting the cascade at all.
|
@mirisuzanne thanks. cleaned this up in 4fb757a |
Change into the src/dialog directory
Run npx http-server .
Open http://localhost:8080/